-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix state bugs in active file analysis #52807
Conversation
What bug does this change fix? |
We've had occasional reports of Current File analysis scope not behaving as expected, but without clear repro steps. While reviewing uses of this internally (mostly during work on #52789), I found cases where state was not correctly updated and caches were populated using information that needs to change when the analysis scope changes. This pull request fixes the cases I found. |
@@ -639,15 +676,17 @@ internal async Task Document_Change(BackgroundAnalysisScope analysisScope, bool | |||
|
|||
var worker = await ExecuteOperation(workspace, w => w.ChangeDocument(document.Id, SourceText.From("//"))); | |||
|
|||
var expectedDocumentEvents = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why all these changes to tests? Do we now expect the same events in all the analysis modes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Incremental analyzers are now responsible for choosing whether or not to act (and the scope of actions) for each analysis mode. Previously this was the case for 2 of the 3 analysis scopes, but now it's a uniform concept.
// change it to check active file (or visible files), not open files if active file tracking is enabled. | ||
// otherwise, use open file. | ||
return document.IsOpen() && document.SupportsDiagnostics(); | ||
if (SolutionCrawlerOptions.GetBackgroundAnalysisScope(document.Project) == BackgroundAnalysisScope.ActiveFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So active file analysis scope is no longer a solution crawler concept for all incremental analyzers, but just something that diagnostic incremental analyzer respects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The same already applied to OpenFilesAndProjects
and FullSolution
.
@@ -77,7 +77,7 @@ private partial class IncrementalAnalyzerProcessor | |||
} | |||
|
|||
// event and worker queues | |||
_documentTracker = _registration.Workspace.Services.GetService<IDocumentTrackingService>(); | |||
_documentTracker = _registration.Workspace.Services.GetRequiredService<IDocumentTrackingService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was possible to be null in the OOP process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ It is no longer possible to be null. A default implementation is provided in 126947c in which no document is ever active. Note that OOP likely needs to provide an implementation of this service which either uses a brokered service to get the current value, or throws with the intent to reveal cases where a "stateless" OOP operation is attempting to rely on state that was not captured in the request.
Given the race conditions in the current version of implementation, I think it would be fine to take this approach. |
Review commit-by-commit recommended